[SDK-400] Validate url, secret, and topic list on webhook creation. #1281
Conversation
sfendell-labelbox
force-pushed
the
sfendell/SDK-400
branch
from
d1de339 to
1ef4ddf
Compare
sfendell-labelbox
force-pushed
the
sfendell/SDK-400
branch
from
1ef4ddf to
f4940b7
Compare
sfendell-labelbox
requested review from
a team,
kopreschko,
mnoszczak,
vbrodsky and
yamini-labelbox
| Information on configuring your server can be found here (this is where the url points to and the secret is set). | ||
| https://docs.labelbox.com/en/configure-editor/webhooks-setup#setup-steps | ||
| https://docs.labelbox.com/reference/webhook |
There was a problem hiding this comment.
A little out of scope, but this docs link is broken.
kopreschko
left a comment
kopreschko
left a comment
There was a problem hiding this comment.
Nice work in both the API and SDK by adding tests for these validations! Just some minor observations, the rest looks good to me.
|
|
||
| webhook.delete() | ||
|
|
||
|
|
There was a problem hiding this comment.
These, in my opinion, should be considered unit-tests.
There was a problem hiding this comment.
There's a lot of tests in integration that could be better considered a unit test, but this needs a configured project and client which only exist in integration fixtures, and pulling them out would be too much of a pain for this I think.
There was a problem hiding this comment.
client and project could be mocks, the exceptions are thrown before these are used.
There was a problem hiding this comment.
Good point. I moved them to unit.
There was a problem hiding this comment.
Second Klaus comments, after that can approve
There was a problem hiding this comment.
It should be fixed now and checks are passing, can I get an approval?
Check that these fields are non-empty on creation to match frontend behavior.